Fix SurfaceMountingManager leaking views from stopped surfaces #37964
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facebook-github-bot
added
CLA Signed
|
This pull request was exported from Phabricator. Differential Revision: D46840717 |
javache
added a commit
to javache/react-native
that referenced
this pull request
Jun 20, 2023
…ook#37964) Summary: Pull Request resolved: facebook#37964 When a Surface is stopped, we don't immediately destroy the SurfaceMountingManager but instead just tear down its internal state. This allows for better error handling (eg did this react tag ever exist, or is this non-existing tag). The way we construct the set of tags post-deletion is flawed though: `mTagToViewState.keySet()` does not create a new Set with all the tags used, but instead uses the underlying HashMap to iterate over the keys as needed. This effectively keeps all the Views inside that deleted surface alive. Changelog: [Android][Fixed] Surfaces in the new architecture no longer leak views once stopped Reviewed By: sammy-SC, rshest Differential Revision: D46840717 fbshipit-source-id: aafab0e550bdfd19e561e282c5baba81e97d2082
javache
force-pushed
the
export-D46840717
branch
from
577befc to
6a6ab6a
Compare
|
This pull request was exported from Phabricator. Differential Revision: D46840717 |
javache
added a commit
to javache/react-native
that referenced
this pull request
Jun 20, 2023
…ook#37964) Summary: Pull Request resolved: facebook#37964 When a Surface is stopped, we don't immediately destroy the SurfaceMountingManager but instead just tear down its internal state. This allows for better error handling (eg did this react tag ever exist, or is this non-existing tag). The way we construct the set of tags post-deletion is flawed though: `mTagToViewState.keySet()` does not create a new Set with all the tags used, but instead uses the underlying HashMap to iterate over the keys as needed. This effectively keeps all the Views inside that deleted surface alive. Changelog: [Android][Fixed] Surfaces in the new architecture no longer leak views once stopped Reviewed By: sammy-SC, rshest Differential Revision: D46840717 fbshipit-source-id: 9fca7af8ec43be410655c747ac6915d0e1f169b3
javache
force-pushed
the
export-D46840717
branch
from
6a6ab6a to
59a9f36
Compare
|
This pull request was exported from Phabricator. Differential Revision: D46840717 |
Base commit: 936936c |
javache
added a commit
to javache/react-native
that referenced
this pull request
Jun 20, 2023
…ook#37964) Summary: Pull Request resolved: facebook#37964 When a Surface is stopped, we don't immediately destroy the SurfaceMountingManager but instead just tear down its internal state. This allows for better error handling (eg did this react tag ever exist, or is this non-existing tag). The way we construct the set of tags post-deletion is flawed though: `mTagToViewState.keySet()` does not create a new Set with all the tags used, but instead uses the underlying HashMap to iterate over the keys as needed. This effectively keeps all the Views inside that deleted surface alive. Changelog: [Android][Fixed] Surfaces in the new architecture no longer leak views once stopped Reviewed By: sammy-SC, rshest Differential Revision: D46840717 fbshipit-source-id: 925cd526e79ed9152ee0c1b289cded50163c1393
javache
force-pushed
the
export-D46840717
branch
from
59a9f36 to
e6bb7af
Compare
|
This pull request was exported from Phabricator. Differential Revision: D46840717 |
…ook#37964) Summary: Pull Request resolved: facebook#37964 When a Surface is stopped, we don't immediately destroy the SurfaceMountingManager but instead just tear down its internal state. This allows for better error handling (eg did this react tag ever exist, or is this non-existing tag). The way we construct the set of tags post-deletion is flawed though: `mTagToViewState.keySet()` does not create a new Set with all the tags used, but instead uses the underlying HashMap to iterate over the keys as needed. This effectively keeps all the Views inside that deleted surface alive. Changelog: [Android][Fixed] Surfaces in the new architecture no longer leak views once stopped Reviewed By: sammy-SC, rshest Differential Revision: D46840717 fbshipit-source-id: 582e55203ae706e6c86eec1f34ad42b10abd50f6
|
This pull request was exported from Phabricator. Differential Revision: D46840717 |
javache
force-pushed
the
export-D46840717
branch
from
e6bb7af to
ea160cd
Compare
|
This pull request has been merged in c16e993. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
When a Surface is stopped, we don't immediately destroy the SurfaceMountingManager but instead just tear down its internal state. This allows for better error handling (eg did this react tag ever exist, or is this non-existing tag).
The way we construct the set of tags post-deletion is flawed though:
mTagToViewState.keySet()does not create a new Set with all the tags used, but instead uses the underlying HashMap to iterate over the keys as needed. This effectively keeps all the Views inside that deleted surface alive.Changelog: [Android][Fixed] Surfaces in the new architecture no longer leak views once stopped
Differential Revision: D46840717